PR Severity: CRITICAL

Highest-severity file determines level | 4 non-test files | 467 lines changed

Critical (2 files):

• htlcswitch/held_htlc_set.go - HTLC forwarding state machine
• htlcswitch/interceptable_switch.go - HTLC forwarding/payment routing interceptable switch

Medium (1 file):

• witness_beacon.go - Root-level Go file

Low (4 files - excluded from severity bump counts):

• docs/release-notes/release-notes-0.21.1.md
• htlcswitch/held_htlc_set_test.go
• itest/list_on_test.go
• itest/lnd_forward_interceptor_test.go

Analysis

This PR modifies core htlcswitch package files. Changes to held_htlc_set.go and interceptable_switch.go affect the HTLC forwarding and payment routing state machine, warranting expert review.

Bump check: 4 non-test files (threshold >20), 467 lines changed (threshold >500), 1 critical package. No bump applied.

To override, add a severity-override-{critical,high,medium,low} label.
<!-- pr-severity-bot -->

Summary of Changes

Hello, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request refactors the HTLC interception mechanism to accurately manage HTLCs that transition from off-chain forwarding to on-chain resolution, particularly in scenarios involving channel force-closures. By introducing distinct handling for off-chain and on-chain held HTLCs, the changes prevent incorrect auto-failing or misdirection of settlement attempts, thereby enhancing the robustness and reliability of HTLC management within the system.

Highlights

• Differentiated HTLC Holding: The heldHtlcSet now distinguishes between offChainHeld and onChainHeld entries, allowing for different behaviors based on whether an HTLC is in the off-chain forwarding flow or the on-chain contractcourt flow.
• Correct On-Chain HTLC Resolution: On-chain held HTLCs are now settle-only and expire by pruning local interceptor state, preventing issues like premature auto-failing after a restart or incorrect routing of settlement messages to the link mailbox instead of the witness beacon.
• Dynamic HTLC State Transition: If an HTLC is initially held off-chain and then re-offered on-chain (e.g., due to a channel force-close), the stored entry in the heldHtlcSet is replaced to ensure future settlement attempts correctly reach the witness beacon.
• Robust Resolution Logic: HTLC resolution now removes an entry from the held set only after the selected action (settle, fail, resume) successfully completes, improving atomicity and reliability.
• Integration Test Coverage: New integration tests have been added to cover critical scenarios, including settling an on-chain intercepted HTLC after a node restart and settling an HTLC that transitions from off-chain to on-chain without a restart.

/gemini review

/gemini review

I think the promotion path should notify the connected interceptor with the updated on-chain intercept.

Step-by-step case:

1. Bob runs an external HtlcInterceptor service.
2. Alice sends a payment through Bob.
3. Bob intercepts the incoming HTLC off-chain before normal forwarding.
4. Bob sends the interceptor client an off-chain intercept request for the circuit key.
5. That request includes auto_fail_height = incoming_timeout - cltvRejectDelta.
6. The interceptor service records this circuit and deadline.
7. The interceptor service holds the HTLC because it is waiting on external business logic, a recipient, or a preimage source outside lnd.
8. Alice force-closes the incoming Alice-Bob channel while the HTLC is still held.
9. Bob's contractcourt starts resolving the force-closed channel.
10. For the held HTLC, contractcourt subscribes to the witness beacon and waits for a matching preimage.
11. The witness beacon re-offers the same HTLC to the interceptor through the on-chain interceptor path.
12. This new on-chain intercept has the same circuit key but different semantics.
13. Off-chain semantics were: RESUME, FAIL, SETTLE, or hold are possible until auto-fail height.
14. On-chain semantics are: only SETTLE is useful; RESUME and FAIL no longer apply.
15. The on-chain useful deadline is the HTLC refund timeout, not the old off-chain auto-fail height.
16. This PR correctly replaces Bob's internal held entry from off-chain to on-chain.
17. Because the entry was already held, interceptOnChain sets wasHeld == true.
18. Because wasHeld == true, this PR does not send the updated on-chain intercept to the connected interceptor client.
19. The interceptor client still only has the old off-chain request.
20. The interceptor client still believes the relevant deadline is incoming_timeout - cltvRejectDelta.
21. The interceptor client does not learn that the HTLC is now on-chain and settle-only.
22. The interceptor client does not learn that it can still settle until RefundTimeout.
23. The old off-chain auto-fail height arrives.
24. A deadline-aware interceptor client abandons the circuit or stops waiting for the external preimage.
25. Later, before the actual on-chain refund timeout, the preimage becomes available outside lnd.
26. Because the client abandoned the circuit, it does not send SETTLE to Bob's lnd.
27. Bob's contractcourt keeps waiting for the preimage in the witness beacon.
28. Bob's lnd never learns the external preimage.
29. The on-chain HTLC reaches timeout.
30. Alice or the counterparty takes the timeout path.
31. Bob misses the on-chain success claim that would have been valid if the interceptor had sent SETTLE.

Severity: blocking. This is in the PR's target no-restart path. The internal held entry is fixed, but the live interceptor client is left with stale off-chain state. For a pure held-before-forward router this may be a failed payment/opportunity loss, but for an interceptor-backed service with external accounting, or a case where Bob already paid/settled downstream, this can become direct funds loss because Bob fails to claim upstream.

This issue is introduced by this PR because it changes the internal held entry from off-chain to on-chain, but intentionally suppresses the duplicate notification for already-held circuit keys. That duplicate notification carries a real state transition.

Suggested fix: re-send the on-chain packet on promotion. The proto docs already say repeated circuit keys must be handled idempotently.

func (s *InterceptableSwitch) interceptOnChain(fwd InterceptedForward) error {
	if err := s.heldHtlcSet.addOnChain(fwd); err != nil {
		return err
	}

	if s.interceptor != nil {
		s.sendForward(fwd)
	}

	return nil
}

The unit test should use different off-chain and on-chain deadlines and assert that the promoted on-chain packet is delivered:

offChain := newMockInterceptedForward(key, 80)
onChain := newMockOnChainInterceptedForward(key, 100)

require.NoError(t, s.heldHtlcSet.addOffChain(offChain))
require.NoError(t, s.interceptOnChain(onChain))

require.Len(t, intercepted, 1)
require.Equal(t, key, intercepted[0].IncomingCircuit)
require.Equal(t, int32(100), intercepted[0].AutoFailHeight())

I think the promotion path should notify the connected interceptor with the updated on-chain intercept.

So the idea was to not change behavior to much from the client perspective in case they become confused why they get the same fwd package again, and would basically discard it, I think it makes sense to maybe add this with the RPC change which introduces the deadline timeout as requested here: #10921, but I can of course change it, just let me know what your final opinions are.

I think the promotion path should notify the connected interceptor with the updated on-chain intercept.

Updated it but slightly more conservative:

• fresh on-chain hold => notify
• off-chain -> on-chain promotion => notify

But it does not send for:

• duplicate already-on-chain offer

Follow-up/non-blocking robustness note: CancelSubscription now synchronously calls back into the interceptable switch via RemoveOnChainIntercept. I don't think this should block the critical fix in this PR, but it creates a liveness dependency worth tracking or addressing in a follow-up.

Step-by-step scenario:

1. Bob has one or more on-chain held HTLCs in heldHtlcSet.
2. An interceptor client connects or reconnects to Router.HtlcInterceptor.
3. The switch event loop receives the interceptor registration.
4. setInterceptor replays currently held HTLCs synchronously from the switch event-loop goroutine.
5. Replay calls s.heldHtlcSet.forEach(s.sendForward).
6. sendForward calls the registered routerrpc interceptor callback synchronously.
7. The routerrpc callback calls stream.Send(interceptionRequest).
8. The interceptor client is operator-controlled/authenticated, but it can still stall, stop reading, deadlock, or be paused.
9. Once the gRPC/HTTP2 send window is exhausted, stream.Send blocks waiting for the client to read.
10. The single interceptable switch event-loop goroutine is now blocked inside interceptor delivery.
11. An on-chain resolver finishes, times out, or otherwise tears down its witness subscription.
12. The resolver runs preimageSubscription.CancelSubscription().
13. CancelSubscription removes the witness subscriber, then calls p.cancelInterceptor(inKey) synchronously.
14. p.cancelInterceptor is s.interceptableSwitch.RemoveOnChainIntercept.
15. RemoveOnChainIntercept sends the circuit key on the unbuffered onchainInterceptDone channel.
16. That send requires the switch event loop to receive from onchainInterceptDone.
17. The switch event loop cannot receive because it is still blocked in stream.Send.
18. Resolver cleanup is now blocked behind progress of the external interceptor stream.

Consequence: resolver cleanup can be delayed, stale on-chain held entries can remain in memory longer than intended, and a resolver goroutine can remain blocked until the interceptor stream unblocks or the switch shuts down. This is not a remote Lightning peer exploit; it is a robustness issue with an operator-controlled interceptor service.

Severity: medium/low follow-up. The PR fixes the funds-loss path, so I would not block on this, but the cleanup path should ideally not synchronously depend on the same switch loop that performs blocking interceptor delivery.

Minimal follow-up fix shape: decouple resolver cleanup from the synchronous switch-loop rendezvous, for example by making the cancellation callback enqueue cleanup asynchronously:

CancelSubscription: func() {
	p.Lock()

	delete(p.subscribers, clientID)

	close(client.quit)
	p.Unlock()

	go func() {
		err := p.cancelInterceptor(inKey)
		if err != nil {
			srvrLog.Errorf("Cannot remove on-chain "+
				"intercept %v: %v", inKey, err)
		}
	}()
},

A stronger design would decouple stream.Send from the switch event loop or add a dedicated async cleanup queue with explicit shutdown semantics.

Created backport PR for v0.20.x-branch:

• [v0.20.x-branch] Backport #10895: htlcswitch: separate onchain and offchain intercpeted HTLCs cleanly #10933 with remaining conflicts!

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin backport-10895-to-v0.20.x-branch
git worktree add --checkout .worktree/backport-10895-to-v0.20.x-branch backport-10895-to-v0.20.x-branch
cd .worktree/backport-10895-to-v0.20.x-branch
git reset --hard HEAD^
git cherry-pick -x 9b31ba83ef52e2d2800f53564f44b9efd4a75ca3 eb1193f80bbe5f0ec2f6618657f1deada5f6561c 98da7b4a56a75ba2dbf47391d43229fd9696e98a 8909c2fbf5535b81ee18c4f4e7fe0160ca43e725 9c5f32a2ec8eab0760a46c3d0357d4127794425f
git push --force-with-lease

Successfully created backport PR for v0.21.x-branch:

• [v0.21.x-branch] Backport #10895: htlcswitch: separate onchain and offchain intercpeted HTLCs cleanly #10934